r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

Hmm now there are two functions named is_diagnostic_item with slightly different semantics. Would it work to have is_diagnostic_item_ctor? I would think we should always know whether or not to expect a ctor, basically only with ExprKind::Call.

I added them so the automated switch from match_def_path would work. Otherwise the lint needs to know which one to use for each case. The ctor DefId will always be used in an expression or pattern, but for a random DefId it could also be from the item directly.

The distinction between the two is also useless in clippy.

They could be named is_*_item_or_ctor which is what's actually happening, but that's also a cumbersome name.

☔ The latest upstream changes (presumably #7639) made this pull request unmergeable. Please resolve the merge conflicts.

for a random DefId it could also be from the item directly

I don't think we have any cases where generality over the two (ctor or not) is actually needed. We are always expecting one or the other depending on context. And the ctor cases are much less common. So we can put in tcx.parent(ctor_id) for those cases and it shouldn't add up to much.

The distinction between the two is also useless in clippy

That is maybe true, but we still have to deal with the distinction. If we "cover up" the ctor DefIds, making it look like it is the diagnostic item id when it is not, that will lead to surprises when you go to use the DefId with other queries.

I don't think we have any cases where generality over the two (ctor or not) is actually needed.

I was referring to the lint suggestion in this case. At the point match_def_path is called it is just a DefId from an unknown source. I guess just suggesting both options wouldn't be a big deal either for an internal lint.

Ah right. Maybe you could query whether the item has a public constructor to at least narrow down some cases.

The lint has to look up the DefId of the item being checked for, so it's easy enough to see if it's a tuple/unit variant. Should I just suggest reworking the code to use is_lang_ctor or is_diagnostic_ctor (and add that one) for those? Otherwise the current functions would need to stay and get renamed.

First attempt at handling ctor suggestions differently.

is_diagnostic_item_or_ctor and is_lang_item_or_ctor are used for structs with publicly accessible constructors as there's no (not incredibly difficult) way to distinguish where a DefId came from.

is_diagnostic_ctor is the counterpart to is_lang_item. There aren't any uses of it, but it's needed to provide a suggestion.

☔ The latest upstream changes (presumably #8219) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #8468) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #8939) made this pull request unmergeable. Please resolve the merge conflicts.

The only extra function introduced now is is_res_diagnostic_ctor which is unused because there aren't any diagnostic item variants currently.

☔ The latest upstream changes (presumably #8666) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #9279) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #8696) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #9573) made this pull request unmergeable. Please resolve the merge conflicts.

r? @llogiq

It looks like @giraffate is either busy or forgot about this.

Dogfood says no:

2022-10-02T13:08:48.0943209Z     Checking clippy_lints v0.1.66 (/home/runner/work/rust-clippy/rust-clippy/clippy_lints)
2022-10-02T13:08:48.0943818Z error: dereferencing a slice pattern where every element takes a reference
2022-10-02T13:08:48.0944250Z   --> src/methods/manual_ok_or.rs:25:41
2022-10-02T13:08:48.0944504Z    |
2022-10-02T13:08:48.0957818Z 25 |         if let ExprKind::Call(err_path, &[ref err_arg]) = or_expr.kind;
2022-10-02T13:08:48.0958204Z    |                                         ^^^^^^^^^^^^^^
2022-10-02T13:08:48.0958444Z    |
2022-10-02T13:08:48.0959050Z    = note: `-D clippy::needless-borrowed-reference` implied by `-D clippy::all`
2022-10-02T13:08:48.0959790Z    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference
2022-10-02T13:08:48.0960246Z help: try removing the `&` and `ref` parts
2022-10-02T13:08:48.0960518Z    |
2022-10-02T13:08:48.0960949Z 25 -         if let ExprKind::Call(err_path, &[ref err_arg]) = or_expr.kind;
2022-10-02T13:08:48.0961336Z 25 +         if let ExprKind::Call(err_path, [err_arg]) = or_expr.kind;

Oops. Looks like I didn't push when I fixed that.

LGTM.

Thank you, this approach seems a clear improvement.

@bors r+

📌 Commit 162aa19 has been approved by llogiq

It is now in the queue for this repository.

⌛ Testing commit 162aa19 with merge bef93d3...

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing bef93d3 to master...